New uniqueness feature by iorveth · Pull Request #1064 · Joystream/joystream

iorveth · 2020-07-27T18:45:05Z

The scope of this PR includes:

New uniqueness feature core logic implementation.
Existing uniqueness failure cases coverage upgrade.
Uniqueness feature failure cases coverage for vector level operations.
Inbound entities reference counting logic related bug fix.
Uniqueness feature enhancement - store only property value hashes instead of actual values.
Uniqueness feature - store only property value hashes instead of actual values #1123

Closes: #1123, #1021
To minimize merge conflict potential, this PR assumes #1035 will be merged first.

…ing logic

…oduce SimplifiedOutputPropertyValue to store property values unique on Class level

bedeho

Seems like removal from index was missed a few places, that needs to be added, and tests added that would trigger on failure. Perhaps update tests to fail frist?
Note that the network-integration-test is not for any sort of unit testing, even in ther untime, its explicitly for the network wide integration tests which Gleb & Mokhtar currently are working on, it does not apply to this PR, so I removed the label.

bedeho · 2020-08-04T13:14:26Z

runtime-modules/content-directory/src/lib.rs

+            //
+
+            // Update property values, that should be unique on Class level
+            Self::add_unique_property_values(class_id, new_unique_property_values);


I believe that there is cleanup processing missing that removes possible old values from the uniqueness index. The tests should be updated to catch this case, and all instances of such missing logic across the extrinsic should be considered.

Only default non required unique value hashes can be removed on update_entity_property_values operation. Implemented here 883863b#diff-0a165b042aa89a04d896ad36b8a33af5R1027
Additional test coverage for the previously discussed failure case fb19f62

bedeho · 2020-08-04T13:19:41Z

runtime-modules/content-directory/src/lib.rs

            let empty_property_value_vector = Self::clear_property_vector(property_value_vector);

+            // Ensure provided `Property` with `unique` flag set is `unique` on `Class` level
+            Self::ensure_property_unique_option_satisfied(class_id, in_class_schema_property_id, &empty_property_value_vector.simplify())?;


This is basically a check that there is no empty vector for this, possible, unique property. Lets update the comment to say exactly that.

bedeho · 2020-08-04T13:27:21Z

runtime-modules/content-directory/src/lib.rs

+            // == MUTATION SAFE ==
+            //
+
+            // Decrease reference counters of involved entities (if some)


Seems to be missing the same cleanup of old value from uniquness index.

Only default non required unique value hashes can be removed, otherwise we only update its respective hash

bedeho · 2020-08-04T13:28:02Z

runtime-modules/content-directory/src/lib.rs

-                .get_vec_value()
-                .get_involved_entities()
-                .and_then(|involved_entities| involved_entities.get(index_in_property_vector as usize).copied());
+            // Insert updated propery value into entity_property_values mapping at in_class_schema_property_id.


Same cleanup missing here.

Only default non required unique value hashes can be removed, otherwise we only update its respective hash.

bedeho · 2020-08-04T13:28:25Z

runtime-modules/content-directory/src/lib.rs

            // == MUTATION SAFE ==
            //

+            // Insert updated property value into entity_property_values mapping at in_class_schema_property_id.


Only default non required unique value hashes can be removed, otherwise we only update its respective hash.

…al values

iorveth · 2020-08-11T11:07:53Z

Uniqueness feature enhancement - store only property value hashes instead of actual values.

#1123

Implementation

Core logic - e03886b
Default non required values potential bug fixed 883863b.

bedeho

This basically looks correct now, well done!

Just one comment about a minor refactor possibility for clarity, and then what we discussed about loud failure and asserting no error in extrinsics.

bedeho · 2020-08-14T13:16:23Z

runtime-modules/content-directory/src/lib.rs

+
+    /// Compute old and new unique property value hashes.
+    /// Ensure new property value hashes with `unique` flag set are `unique` on `Class` level
+    pub fn ensure_property_values_hashes(


This ensure checkers has a name that does not really indicate a clear purpose.

Closer inspection shows that it seems to do three things

check that new property values respect uniquness constraint

if so, compute hashes representing these values values

if so, determine what hashes must be removed from the uniquness index because they represent values that will no longer be in directory.

It seems that only 1 or 2 are really part of the ensure check, and you could perhaps have one checker with a name like ensure_new_property_values_respect_uniquness. It can then return hashes in success case. Then for 3, which is only relevant if you go to mutation, you do the call to compute_old_unique_hashes somewhere else. Could be a standalone call, or it could be inside remove_unique_property_value_hashes or something else.

Agree, refactored it here 673fcb3
Thank you for the good explanation!

iorveth added 4 commits July 25, 2020 09:47

New uniqueness feature core logic implemented

d9a51a8

cargo fmt

88a6660

Update existing uniqueness test coverage, fix inbound reference count…

84d7782

…ing logic

Tests: vector level operations uniqueness feature test coverage, intr…

b54cd57

…oduce SimplifiedOutputPropertyValue to store property values unique on Class level

iorveth self-assigned this Jul 27, 2020

iorveth added content-pallet network-integration-test End-to-end full network integration test bug Something isn't working and removed bug Something isn't working labels Jul 27, 2020

iorveth requested a review from bedeho July 27, 2020 18:48

iorveth mentioned this pull request Jul 28, 2020

substrate 2.0.0-rc4 version upgrade #1072

Merged

iorveth changed the base branch from content_directory_second_try to content_dir_2 July 31, 2020 13:28

iorveth changed the base branch from content_dir_2 to content_directory_second_try July 31, 2020 13:28

bedeho changed the base branch from content_directory_second_try to master August 4, 2020 09:34

bedeho changed the base branch from master to content_dir_2 August 4, 2020 09:34

bedeho changed the base branch from content_dir_2 to content_directory_second_try August 4, 2020 09:36

bedeho removed the network-integration-test End-to-end full network integration test label Aug 4, 2020

bedeho requested changes Aug 4, 2020

View reviewed changes

iorveth added 6 commits August 10, 2020 17:15

Uniqueness feature - store only property value hashes instead of actu…

e03886b

…al values

Tests: Additional failure case for the uniqueness feature

fb19f62

apply cargo fmt

44fa3b7

Improve compute_unique_hashes implementation

0a2d3f8

Uniqueness feature: fix potential issue with default non required values

883863b

Typo fixed

5623fa8

iorveth requested a review from bedeho August 11, 2020 11:09

iorveth added 4 commits August 12, 2020 11:33

Fix uniqueness feature implementation issue

9815057

Refactoring: uniqueness feature

1da9b94

Renaming: Output -> Stored

2afe212

Make OutputValueForExistingProperty::from method infallible

1b744b7

Fix clippy

f0c1f0d

bedeho requested changes Aug 14, 2020

View reviewed changes

iorveth added 2 commits August 14, 2020 22:31

ensure_new_property_values_respect_uniquness method refactoring

673fcb3

Use debug_assert macro, when from method conversion should not fail

828eb8d

iorveth requested a review from bedeho August 15, 2020 00:11

bedeho approved these changes Aug 15, 2020

View reviewed changes

bedeho merged commit 5000ca8 into Joystream:content_directory_second_try Aug 15, 2020

This was referenced Aug 15, 2020

Uniqueness feature - store only property value hashes instead of actual values #1123

Closed

Unique feature reworking #1021

Closed

Conversation

iorveth commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedeho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iorveth Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iorveth commented Aug 11, 2020

Uniqueness feature enhancement - store only property value hashes instead of actual values.

Implementation

Uh oh!

bedeho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

iorveth commented Jul 27, 2020 •

edited

Loading

iorveth Aug 11, 2020 •

edited

Loading